Skip to content

Add core SSL trust store resolution and fallback logic (PR 2 of 3)#786

Merged
madhav-db merged 25 commits into
mainfrom
ssl-truststore-handling-2
Apr 25, 2025
Merged

Add core SSL trust store resolution and fallback logic (PR 2 of 3)#786
madhav-db merged 25 commits into
mainfrom
ssl-truststore-handling-2

Conversation

@madhav-db
Copy link
Copy Markdown
Collaborator

@madhav-db madhav-db commented Apr 11, 2025

Description

This PR introduces the baseline SSL trust-store resolution logic and associated tests, enabling basic support for loading custom trust stores and handling fallbacks.

Flow explanation: https://docs.google.com/document/d/1TSfUsci3n9dXzXcnaYCNG7i7bmQo-PkXBQY1QrHzkpg/edit?tab=t.0#heading=h.ofrwhzeixs4q

Changes

  • Add SocketFactoryUtil for test and self-signed certs
  • Implement basic ConfiguratorUtils logic for:
    • loading custom trust stores
    • falling back to system and JDK default trust stores
  • Add SSLTest for direct/proxy connections with trust store support
  • Add minimal unit tests in ConfiguratorUtilsTest for baseline paths

Testing

Tested with the SSLTest integration suite and unit test coverage in ConfiguratorUtilsTest.

Additional Notes to the Reviewer

This is PR 2 of 3 for the SSL configuration enhancement work.

NO_CHANGELOG=true
Will add changelog in last PR.

@madhav-db madhav-db changed the title Ssl truststore handling 2 Enhance SSL Trust Store Handling with Improved Error Handling Apr 11, 2025
@madhav-db madhav-db changed the title Enhance SSL Trust Store Handling with Improved Error Handling Enhance SSL Trust Store Handling with Improved Error Handling (PR 2 of 3) Apr 11, 2025
Copy link
Copy Markdown
Collaborator

@vikrantpuppala vikrantpuppala left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry but there are severe logic flaws in this code, can we please manually test all flows that we're making changes to? I am afraid I will miss out something in one of these reviews and we're going to push out regressions.

Comment thread src/main/java/com/databricks/jdbc/dbclient/impl/common/ConfiguratorUtils.java Outdated
Comment thread src/main/java/com/databricks/jdbc/dbclient/impl/common/ConfiguratorUtils.java Outdated
Comment thread src/main/java/com/databricks/jdbc/dbclient/impl/common/ConfiguratorUtils.java Outdated
Comment thread src/main/java/com/databricks/jdbc/dbclient/impl/common/ConfiguratorUtils.java Outdated
Comment thread src/main/java/com/databricks/jdbc/dbclient/impl/common/ConfiguratorUtils.java Outdated
Comment thread src/main/java/com/databricks/jdbc/dbclient/impl/common/ConfiguratorUtils.java Outdated
Comment thread src/main/java/com/databricks/jdbc/dbclient/impl/common/ConfiguratorUtils.java Outdated
Comment thread src/main/java/com/databricks/jdbc/dbclient/impl/common/ConfiguratorUtils.java Outdated
Comment thread src/main/java/com/databricks/jdbc/dbclient/impl/http/DatabricksHttpClient.java Outdated
@madhav-db madhav-db merged commit 89dd484 into databricks:main Apr 25, 2025
14 of 16 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants